Add TypeScript generics for component props #11731

nickserv · 2018-06-06T01:41:40Z

This should allow strongly typing extra props derived from component props, such as using the to prop of React Router's Link component on ButtonBase components. Prop interfaces that include a component prop with prop spreading now have a generic type with no constraint that default to an empty interface. This also makes it possible to use TypeScript 2.9's new JSX generics when they can't be inferred.

Examples

<Button component={Link} to="/open-collective">Link</Button>
<Button<LinkProps> component={Link} to="/open-collective">Link</Button>

That's definitely an improvement over what I was using, though I like the type safety and convenience of directly checking props on the component's generic type without having to create a separate component to work around TypeScript. Also with @oliviertassinari's example it seems like you have to import { ListItemProps } from '@material-ui/core/ListItem' because the props aren't included in index.d.ts, it would be nice to export them if we don't take the generic approach.

nickserv · 2018-06-07T01:24:27Z

Actually I am having an issue with that approach, the following code errors because ListItem uses a string ref while Link uses a function ref for innerRef: const NameLink = (props: ListItemProps) => <Link to={name} {...props} />

Is there a way to strongly type this without generics (not using any)?

franklixuefei · 2018-06-22T20:06:05Z

What's the status of this PR? Seems related to #11921

pelotom · 2018-06-22T20:24:07Z

It's a fairly extensive change and I've been too busy to give it the attention it deserves, might get some time to review this weekend. Looks like it has conflicts currently though.

nickserv · 2018-06-22T23:43:20Z

This should be up to date now.

pelotom

This is really promising! I wasn't aware that inference of component type arguments would be good enough to make this kind of approach work.

pelotom · 2018-06-24T22:25:09Z

packages/material-ui/test/typescript/components.spec.tsx

+    <Button<LinkProps> component={Link} to="/open-collective">
+      Link
+    </Button>
+    <Button<LinkProps> to="/open-collective">Link</Button>


It looks like the generic argument can actually be left off here and inference works fine:

- <Button<LinkProps> component={Link} to="/open-collective"> + <Button component={Link} to="/open-collective"> Link </Button> - <Button<LinkProps> to="/open-collective">Link</Button> + <Button to="/open-collective">Link</Button>

pelotom · 2018-06-24T23:51:16Z

packages/material-ui/src/Avatar/Avatar.d.ts

@@ -14,6 +14,6 @@ export interface AvatarProps

 export type AvatarClassKey = 'root' | 'colorDefault' | 'img';

-declare const Avatar: React.ComponentType<AvatarProps>;
+declare class Avatar<C> extends React.Component<C & AvatarProps<C>> {}

 export default Avatar;


It seems like what we actually want here (and similarly for other components) is something more like

export interface AvatarProps<C = React.HTMLAttributes<HTMLDivElement>> extends StandardProps<C, AvatarClassKey> & {

I.e. in each case take the type we used to be passing as the first argument to StandardProps, make that the default for C, and then pass C to StandardProps instead. There's a problem though: we can no longer use an interface because we're intersecting with a type variable, so we have to switch to a type alias and intersection:

export type AvatarProps<C = React.HTMLAttributes<HTMLDivElement>> = StandardProps<C, AvatarClassKey> & {

Also StandardProps does the work of &-ing that first argument with the rest of your props, so you don't need to do it in the declare statement:

declare class Avatar<C> extends React.Component<AvatarProps<C>> {}

This approach looks promising at first blush, but it doesn't quite work ideally because the default type argument isn't used when you use a bare component, i.e. this fails:

<Avatar title="hello" /> // `title` from HTMLAttributes is invalid

You have to provide an explicit type parameter to fix it:

<Avatar<React.HTMLAttributes<HTMLDivElement>> title="hello" />

This would be a huge breaking change and is just gross 😕

But conditional types to the rescue! This seems to work:

export type AvatarProps<C = {}> = StandardProps< {} extends C ? React.HTMLAttributes<HTMLDivElement> : C, AvatarClassKey > & {

Basically, if C is not inferred as anything specific, either because you referenced bare AvatarProps or because you didn't provide a component prop to Avatar to allow it to disambiguate, you will get the prop types that match the default component, in this case a div.

Now we can just write

<Avatar title="hello" />

and importantly, this doesn't work, which it shouldn't!

<Avatar component={(props: {/* no title prop! */}) => <div />} title="hello" />

What do you think?

pelotom · 2018-07-05T07:01:02Z

I tried out the above strategy on a local copy of this branch and it almost works, but there's a pretty unfortunate limitation: it turns out TypeScript can't infer the parameter types of callback props associated with the inner component. So for example, all the basic event handler props e.g. onClick associated with a default component of div would no longer be able to infer the parameter type and would require annotation.

I've filed a TypeScript ticket and asked a question on SO about it.

nickserv · 2018-07-05T08:01:36Z

Thanks for your research and suggestions. Is that a back compatibility breaking issue that would need to be fixed before we continue?

pelotom · 2018-07-05T08:12:55Z

@nickmccurdy

Is that a back compatibility breaking issue that would need to be fixed before we continue?

Yes, it would be backwards-breaking. To make it concrete, currently you can write this

<Button onClick={e => console.log(e.target)}>
  Click me!
</Button>

and TypeScript infers the type of the e parameter just fine. But with this change it would give up and infer e: any unless you provide an explicit annotation like

<Button onClick={(e: React.MouseEvent<HTMLElement>) => console.log(e.target)}>
  Click me!
</Button>

On the plus side if you have noImplicitAny enabled it would give you an error if you provide anything but the correct type annotation, which is more than can be said for the current situation, but it's quite a bit less convenient 😕

oliviertassinari · 2018-07-13T08:59:36Z

How do you guys want to move forward? Should we just commit the additional tests with this pull request?

pelotom · 2018-07-13T15:53:18Z

@oliviertassinari I was really hoping we'd find a way around the issue with inferring callback parameter types, but my bounty on the SO question hasn't yielded any new answers, so I'm losing hope :( I think if we could get it so that everything works as-is for the case where you're not overriding the component, and then if you do override the component you might need to provide more type information, e.g.

// works with no additional annotation:
<Avatar onClick={e => log(e)} alt="Image Alt" src="example.jpg" />
// requires annotating with either component or props type:
<Avatar<'button'> onClick={e => log(e)} component="button" alt="Image Alt" src="example.jpg" />
<Avatar<{ x: number }>
  component={(props: { x: number }) => <div />}
  x={3}
  // Should NOT be allowed:
  // onClick={e => log(e)}
  alt="Image Alt"
  src="example.jpg"
/>
// OR, requires giving parameter types for callbacks:
<Avatar onClick={(e: React.MouseEvent<HTMLButtonElement>) => log(e)} component="button" alt="Image Alt" src="example.jpg" />

then that would still be a huge win. This seems more feasible somehow, but I still haven't figured out a way to do it.

If the PR is viewed as clutter, we can close it, but I maintain hopes that it (or something like it) can be merged some day.

As far as merging the tests, some of them won't pass without the rest of the PR.

oliviertassinari · 2018-07-15T10:35:12Z

@pelotom Thanks for the follow up. Do as you see fit with this pull request.

pelotom · 2018-08-14T23:13:25Z

I've been playing around with a new strategy for solving this problem. It looks promising, but I've run into another TypeScript bug: microsoft/TypeScript#26004. Anyone interested in seeing this accomplished should go put their 👍 on it!

nickserv · 2018-08-14T23:40:16Z

Thanks for continuing to look into this, I'll track the upstream issues. This is still blocked by TypeScript, right? Let me know if I can do anything.

pelotom · 2018-08-14T23:44:32Z

This is still blocked by TypeScript, right?

@nickmccurdy yes, in particular by microsoft/TypeScript#26004 now.

oliviertassinari · 2018-10-02T21:28:20Z

I'm closing the pull request for now. Feel free to reopen it if any progress is made :). Thank you guys for the effort!

kossmoboleat · 2018-12-05T10:27:01Z

Seems like Microsoft closed the issue because it was fixed in a PR and that the new version 3.2.1 was also released:
microsoft/TypeScript#27408

pelotom · 2018-12-05T16:57:26Z

Thanks @kossmoboleat, I'll take another look at this soon!

pelotom · 2018-12-10T01:51:50Z

I've opened a new PR at #13868.

Testing #11731

nickserv added 4 commits June 4, 2018 22:42

Add TypeScript generics for component props

343c33a

Add default generics

6a9b053

Fix Prettier parser and format TypeScript files

b012412

Revert formatting on unrelated files to reduce noise

6d25a43

oliviertassinari added the typescript label Jun 6, 2018

oliviertassinari requested review from pelotom June 6, 2018 06:43

nickserv added 3 commits June 22, 2018 17:22

Merge branch 'master' into typescript-generics

b48821d

Fix syntax error in generic

36c77f3

Add generic to component prop of Tabs

8d381d5

pelotom suggested changes Jun 24, 2018

View reviewed changes

pelotom added 3 commits July 1, 2018 11:34

Merge branch 'master' into typescript-generics

9187839

Remove unnecessary generic arguments

4265490

Add PassthruProps operator and use it in AvatarProps

fd5acad

pelotom mentioned this pull request Jul 5, 2018

Failure to infer parameter type microsoft/TypeScript#25444

Closed

Add failing tests

8adae04

oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Jul 8, 2018

pelotom mentioned this pull request Jul 10, 2018

[docs] TypeScript Guide #8598

Closed

pelotom mentioned this pull request Aug 4, 2018

[ToggleButtonGroup] make value type generic #12332

Closed

pelotom mentioned this pull request Aug 13, 2018

ListItem doesn't accept prop htmlFor (TypeScript) #12486

Closed

2 tasks

pelotom mentioned this pull request Aug 22, 2018

ListItemIcon missing onClick event in typescript definition ( ListItemIcon.d.ts ) #12534

Closed

pelotom mentioned this pull request Aug 30, 2018

[typescript] Allow custom props for custom inputComponent #12697

Closed

oliviertassinari force-pushed the master branch 2 times, most recently from 5db2370 to a4b65b7 Compare September 21, 2018 21:17

oliviertassinari closed this Oct 2, 2018

This was referenced Oct 8, 2018

[typescript] Fix onChange event types to React.ChangeEvent<HTMLInputElement> #13065

Closed

[TypeScript] Error when using the component-property on Buttons with react-router-dom #13218

Closed

pelotom mentioned this pull request Dec 10, 2018

[typescript] Enable generic props for certain components #13868

Merged

eps1lon mentioned this pull request Feb 10, 2019

[docs] Add simple list typescript demo #14485

Merged

eps1lon added a commit that referenced this pull request Mar 2, 2019

[docs] Add simple list typescript demo (#14485)

ecc2c85

Testing #11731

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TypeScript generics for component props #11731

Add TypeScript generics for component props #11731

nickserv commented Jun 6, 2018 •

edited

Loading

nickserv commented Jun 6, 2018

oliviertassinari commented Jun 6, 2018

nickserv commented Jun 6, 2018 •

edited

Loading

nickserv commented Jun 7, 2018 •

edited

Loading

franklixuefei commented Jun 22, 2018

pelotom commented Jun 22, 2018 •

edited

Loading

nickserv commented Jun 22, 2018

pelotom left a comment

pelotom Jun 24, 2018

pelotom Jun 24, 2018 •

edited

Loading

pelotom commented Jul 5, 2018

nickserv commented Jul 5, 2018

pelotom commented Jul 5, 2018 •

edited

Loading

oliviertassinari commented Jul 13, 2018 •

edited

Loading

pelotom commented Jul 13, 2018

oliviertassinari commented Jul 15, 2018

pelotom commented Aug 14, 2018

nickserv commented Aug 14, 2018 •

edited

Loading

pelotom commented Aug 14, 2018

oliviertassinari commented Oct 2, 2018 •

edited

Loading

kossmoboleat commented Dec 5, 2018

pelotom commented Dec 5, 2018

pelotom commented Dec 10, 2018

Add TypeScript generics for component props #11731

Add TypeScript generics for component props #11731

Conversation

nickserv commented Jun 6, 2018 • edited Loading

Examples

Related

nickserv commented Jun 6, 2018

oliviertassinari commented Jun 6, 2018

nickserv commented Jun 6, 2018 • edited Loading

nickserv commented Jun 7, 2018 • edited Loading

franklixuefei commented Jun 22, 2018

pelotom commented Jun 22, 2018 • edited Loading

nickserv commented Jun 22, 2018

pelotom left a comment

Choose a reason for hiding this comment

pelotom Jun 24, 2018

Choose a reason for hiding this comment

pelotom Jun 24, 2018 • edited Loading

Choose a reason for hiding this comment

pelotom commented Jul 5, 2018

nickserv commented Jul 5, 2018

pelotom commented Jul 5, 2018 • edited Loading

oliviertassinari commented Jul 13, 2018 • edited Loading

pelotom commented Jul 13, 2018

oliviertassinari commented Jul 15, 2018

pelotom commented Aug 14, 2018

nickserv commented Aug 14, 2018 • edited Loading

pelotom commented Aug 14, 2018

oliviertassinari commented Oct 2, 2018 • edited Loading

kossmoboleat commented Dec 5, 2018

pelotom commented Dec 5, 2018

pelotom commented Dec 10, 2018

nickserv commented Jun 6, 2018 •

edited

Loading

nickserv commented Jun 6, 2018 •

edited

Loading

nickserv commented Jun 7, 2018 •

edited

Loading

pelotom commented Jun 22, 2018 •

edited

Loading

pelotom Jun 24, 2018 •

edited

Loading

pelotom commented Jul 5, 2018 •

edited

Loading

oliviertassinari commented Jul 13, 2018 •

edited

Loading

nickserv commented Aug 14, 2018 •

edited

Loading

oliviertassinari commented Oct 2, 2018 •

edited

Loading